Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update Readme to reflect correct argument for recipeId #72

Merged
merged 1 commit into from
Oct 3, 2024

Conversation

Divs-B
Copy link
Contributor

@Divs-B Divs-B commented Sep 27, 2024

What does this change?

Their was a requirement came up by iOS devs to reindex a recipe. @paulmr and I tried running locally the script as mentioned in Readme and provided recipe id which ended up in an. error TypeError [ERR_PARSE_ARGS_UNKNOWN_OPTION]: Unknown option '--recipe-uid'

We found in the script that actually argument is recipeUid and not recipe-uid and hence changed it to make it work.

How to test

Command should run without any error on recipe id.

How can we measure success?

re index should run

README.md Outdated
@@ -26,26 +27,28 @@ For a tighter feedback loop, run individual projects or files, and watch, with `
## How do I re-index content from CAPI out to Feast?

1. Set up for local operations, as above
2. Run `npm run commandline-reindex -- [--composer-id 1234567] [--capi-uri path/to/article/in/capi] [--recipe-uid 0551534c8d93e8da7bb70553b10fa0d0f62534a3]`
2. Run `npm run commandline-reindex -- [--composer-id 1234567] [--capi-uri path/to/article/in/capi] [--recipeUid 0551534c8d93e8da7bb70553b10fa0d0f62534a3]`
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one line change is actually for the PR others are auto linting i think.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I'm reading this correctly, all of these parameter values are camel case aren't they?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh actually yes, others are also camel case. since I haven't ran any these arguments hence skipped checking them at the first place. Do you think we can amend these as well in this PR?
Or, I hope it doesn't mean that letter should be camel case after single hyphen? like --composer-id should be passed as composerId and I have not checked it?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Honestly I haven't dug into the details. Maybe snake-case params are automagically converted into camelCase somewhere and so it doesn't matter. The important thing is that the README is an accurate reflection of what we need to do. It just struck me as odd that we'd be illustrating a mixture of techniques as it means users will potentially have to refer to the README every time they want to use the script to remind themselves which param should be in which format - if it actually makes a difference, which I assume it does or there would be no need to make this change at all.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, so the news is not good 😞

Running this test script proves that there is no magic, and all the arge in this case are in fact camelCase (no idea how I missed that! sorry!)

#!/usr/bin/env node

const { parseArgs } = require("node:util");

const {values} = parseArgs({
    options: {
        help: {
            type: 'boolean',
            short: 'h'
        },
        testOne: {
            type: 'string',
            short: 't'
        },
        testTwo: {
            type: 'string',
        }
    }
});

console.log(values);
$ node testargs.js --help --testOne Hello --testTwo 'everybody here'
[Object: null prototype] {
  help: true,
  testOne: 'Hello',
  testTwo: 'everybody here'
}
node testargs.js --help --test-one Hello --testTwo 'everybody here'
node:internal/util/parse_args/parse_args:98
    throw new ERR_PARSE_ARGS_UNKNOWN_OPTION(
    ^

TypeError [ERR_PARSE_ARGS_UNKNOWN_OPTION]: Unknown option '--test-one'
    at new NodeError (node:internal/errors:405:5)
    at checkOptionUsage (node:internal/util/parse_args/parse_args:98:11)
    at node:internal/util/parse_args/parse_args:360:9
    at Array.forEach (<anonymous>)
    at parseArgs (node:internal/util/parse_args/parse_args:357:3)
    at Object.<anonymous> (/Users/andy_gallagher/testargs.js:5:18)
    at Module._compile (node:internal/modules/cjs/loader:1256:14)
    at Module._extensions..js (node:internal/modules/cjs/loader:1310:10)
    at Module.load (node:internal/modules/cjs/loader:1119:32)
    at Module._load (node:internal/modules/cjs/loader:960:12) {
  code: 'ERR_PARSE_ARGS_UNKNOWN_OPTION'
}

Behaviour is the same on Node v16.20.2, v18.18.2 and v22.6.0.

@Divs-B please could you update the other arguments in the README? Thanks a lot!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thats a good finding to confirm on this, thankyou 👍 .
I will update Readme for other args too.

@Divs-B Divs-B force-pushed the db/update-readme-to-correct-recipeid-for-commandline branch from 2e6c74d to b40fec0 Compare October 2, 2024 08:45
Copy link
Contributor

@fredex42 fredex42 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😀

@Divs-B Divs-B merged commit 31db10c into main Oct 3, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants